Skip to content

chore(test-types): adds extra modifier validation; adds unit tests#2710

Merged
marioevz merged 4 commits intoethereum:forks/amsterdamfrom
fselmo:chore/validate-modifiers
Apr 22, 2026
Merged

chore(test-types): adds extra modifier validation; adds unit tests#2710
marioevz merged 4 commits intoethereum:forks/amsterdamfrom
fselmo:chore/validate-modifiers

Conversation

@fselmo
Copy link
Copy Markdown
Contributor

@fselmo fselmo commented Apr 17, 2026 •
edited
Loading

🗒️ Description

From comment on EthR&D here, adds missing sanity check validations to BAL modifiers. These are currently not producing any errors but could allow some to slip through if not addressed.

✅ Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks: 
    just static
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

Cute Animal Picture

Screenshot 2026-04-17 at 09 54 37

@fselmo fselmo added C-chore Category: chore A-test-types Area: execution_testing.base_types and execution_testing.test_types labels Apr 17, 2026
@fselmo fselmo force-pushed the chore/validate-modifiers branch from ec3b5b4 to 2d2ec31 Compare April 17, 2026 16:02
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.47%. Comparing base (e8e9c38) to head (e48d4e0).

Additional details and impacted files 
@@               Coverage Diff                @@
##           forks/amsterdam    #2710   +/-   ##
================================================
  Coverage            86.47%   86.47%           
================================================
  Files                  599      599           
  Lines                37632    37632           
  Branches              3795     3795           
================================================
  Hits                 32542    32542           
  Misses                4526     4526           
  Partials               564      564
Flag Coverage Δ
unittests 86.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@felix314159
Copy link
Copy Markdown
Contributor

felix314159 commented Apr 20, 2026

i think there is a state leak bug, added a test to show it. run with

uv run pytest packages/testing/src/execution_testing/test_types/tests/test_block_access_list_modifiers.py -v -s -k reused_callable

feel free to fix

felix314159
felix314159 reviewed Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@felix314159 felix314159 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above, forgot to mark it as review comment

@fselmo fselmo force-pushed the chore/validate-modifiers branch from ba1ba1b to e48d4e0 Compare April 21, 2026 21:26
@fselmo
Copy link
Copy Markdown
Contributor Author

fselmo commented Apr 21, 2026 •
edited
Loading

i think there is a state leak bug, added a test to show it. run with

@felix314159 there sure was, thanks for catching 👌🏼. This was the case in a few places... added some unit tests for that too and patched this up. Ready for a review again 👀 .

Rebased and did a hasher compare as a sanity check -- no difs on the invalid tests compared to forks/amsterdam.

@marioevz marioevz merged commit 49b882e into ethereum:forks/amsterdam Apr 22, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felix314159 felix314159 felix314159 approved these changes

Assignees

No one assigned

Labels

A-test-types Area: execution_testing.base_types and execution_testing.test_types C-chore Category: chore

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fselmo @felix314159 @marioevz